Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Unknown command event #6534

Open
wants to merge 36 commits into
base: dev/feature
Choose a base branch
from

Conversation

7othifaPS
Copy link

@7othifaPS 7othifaPS commented Apr 3, 2024

Adds the [player] unknown command event to skript

with event-value unknown command( |-)message

Related Issue: #1655

@EquipableMC
Copy link
Contributor

i'd merge under dev/feature

@7othifaPS
Copy link
Author

idk why it didn't merge it

@mpschorr
Copy link

mpschorr commented Apr 3, 2024

a team member has to approve it

@7othifaPS
Copy link
Author

how would i mention it to the issue that is related to it

@7othifaPS 7othifaPS mentioned this pull request Apr 4, 2024
14 tasks
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution ⚡

Here are few notes :)

We should also add support for this in ExprCommand and likely add some support in EvtCommand (like adding a new syntax to ignore unknown commands)

@AyhamAl-Ali AyhamAl-Ali changed the base branch from master to dev/feature April 4, 2024 01:56
@AyhamAl-Ali AyhamAl-Ali added the feature Pull request adding a new feature. label Apr 4, 2024
@7othifaPS
Copy link
Author

Thank you for your contribution ⚡

Here are few notes :)

We should also add support for this in ExprCommand and likely add some support in EvtCommand (like adding a new syntax to ignore unknown commands)

oh ight, i'll check notes in the morning

@7othifaPS
Copy link
Author

Done, i did all notes

I put the event in SimpleEvents, and the message in ExprMessage. I committed the changes but how do i show them here

@AyhamAl-Ali
Copy link
Member

Done, i did all notes

I put the event in SimpleEvents, and the message in ExprMessage. I committed the changes but how do i show them here

You need to push them to your fork on branch ilikeskript

@7othifaPS
Copy link
Author

it rejects it for some reason
image

@AyhamAl-Ali
Copy link
Member

it rejects it for some reason image

You need to switch upstream to your fork not origin

@7othifaPS
Copy link
Author

7othifaPS commented Apr 4, 2024

i think i did it?

it should be committed here

Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also add support for event values

src/main/java/ch/njol/skript/expressions/ExprMessage.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/SimpleEvents.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/SimpleEvents.java Outdated Show resolved Hide resolved
@7othifaPS
Copy link
Author

I did the things you said but for the check if class exists, the event isn't paper, its bukkit

@7othifaPS
Copy link
Author

there

Copy link
Member

@Pikachu920 Pikachu920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for your contribution! can you please add an event value for the command sender, and support for ExprCommand/ExprArgument? let me know if you need any help

src/main/java/ch/njol/skript/events/SimpleEvents.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/SimpleEvents.java Outdated Show resolved Hide resolved
@7othifaPS
Copy link
Author

There, tell me if something's wrong

sorry for having all these, its my first time :)

@7othifaPS
Copy link
Author

Oh wait, how should i add a player event-value to the event? since it doesn't have its own class

@7othifaPS 7othifaPS requested a review from AyhamAl-Ali April 18, 2024 16:53
@7othifaPS 7othifaPS requested a review from sovdeeth April 18, 2024 17:09
@AyhamAl-Ali AyhamAl-Ali added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Apr 18, 2024
Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test should still be added

@Moderocky Moderocky removed the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label May 10, 2024
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could have a test.

Copy link
Collaborator

@TheLimeGlass TheLimeGlass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the JUnit have any 'ensure's set up to ensure that the unknown command event gets called and returns what is thought of for the JUnit?

src/main/java/ch/njol/skript/expressions/ExprArgument.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprArgument.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprArgument.java Outdated Show resolved Hide resolved
Comment on lines 1 to 2
on unknown command:
set unknown command message to "Hey, this command does not exist."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test won't ever get called on its own, you need to call the event in the java JUnit package and then ensure that you can modify the unknown command message correctly. look at other expressions for examples

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fire for console commands? You might be able to get away with execute console command "fakecommand" in a test

Comment on lines +1 to +2
on unknown command:
set unknown command message to "Hey, this command does not exist."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.